Backport Proof of Space perf improvements from abundance #3712

teor2345 · 2025-10-15T13:49:13Z

This PR backports the latest compatible Proof of Space changes from abundance, including some API refactors, including all relevant files up until commit:
nazar-pc/abundance@6d56b48

It should be reviewed one commit at a time.
The copied code is best reviewed by:

checking out abundance at that commit
copying across the code from abundance
checking there are no differences

The copied code includes:

ab-proof-of-space replacing subspace-proof-of-space
ab-chacha8 copied into shared
SolutionPotVerifier trait from ab-core-primitives

Various minor fixes are then made to get it to compile and pass CI:

Cargo.toml build files
imports
incompatibilities
revert dependency and compiler changes we haven't updated to yet
clippy/lint fixes
missed docs

I also added some extra tests and benchmark debugging.

Code contributor checklist:

I have read, understood and followed contributing guide

…ace (nazar-pc/abundance#406) Using the entire ab-proof-of-space as at commit: nazar-pc/abundance@6d56b48 And merge changes from ab-proof-of-space/Cargo.toml into subspace-proof-of-space/Cargo.toml.

Using the entire ab-chaha8 as at commit: nazar-pc/abundance@6d56b48

immunefi-magnus · 2025-10-15T13:49:22Z

🛡️ Immunefi PR Reviews

We noticed that your project isn't set up for automatic code reviews. If you'd like this PR reviewed by the Immunefi team, you can request it manually using the link below:

🔗 Send this PR in for review

Once submitted, we'll take care of assigning a reviewer and follow up here.

teor2345 · 2025-10-15T14:06:47Z

The benchmark failure might take some time to fix, and I've run out of time today. Looks like the benchmarks expect Proof of Space to return Ok, but instead it's returning an error.

The "check individually" failure is trivial, just some missing dead code annotations.

nazar-pc

Note that miri annotations may not be necessary since this repo doesn't run tests under Miri (abundance does to have additional guarantees that unsafe code maintains invariants, but some tests are too slow to run regularly in CI or impractical to run in general, hence corresponding annotations).

Similarly no-panic ensures compiler can figure out the code can't possibly panic at compile time, which allows for better optimizations, but this repo doesn't run corresponding tests, so not sure how useful it is to port over.

vedhavyas

Did a review but not confident in my review yet due to big number of changes in the first PR Commit itself.

Ignored tests and benchmarks for this review

Some of the later commits can be merged up as they add no extra benefit for review

vedhavyas · 2025-10-22T06:28:18Z

crates/subspace-proof-of-space/src/chia.rs


+impl ab_core_primitives::solutions::SolutionPotVerifier for ChiaTable {
+    fn is_proof_valid(seed: &PosSeed, challenge_index: u32, proof: &PosProof) -> bool {
+        Tables::<K>::verify_only(seed, challenge_index.to_le_bytes(), proof)


is the behaviour same for verify and verify_only ?

verify follows Chia API and returns the quality back. verify_only doesn't compute the quality, just verification. The verification itself is reused and is identical. You can see the logic in the fn verify, it has exactly one branch.

vedhavyas · 2025-10-22T06:51:16Z

shared/ab-chacha8/src/lib.rs

+
+        // TODO: More idiomatic version currently doesn't compile:
+        //  https://github.com/Rust-GPU/rust-gpu/issues/241#issuecomment-3005693043
+        #[allow(clippy::needless_range_loop)]


confused here with this clippy allowed warning. Is this allow required ?

It is required for GPU implementation, but not for CPU. It is just pulled from the codebase where GPU support is already partially implemented and this was needed to make it both compile and not complain.

teor2345 · 2025-10-22T19:21:54Z

Note that miri annotations may not be necessary since this repo doesn't run tests under Miri (abundance does to have additional guarantees that unsafe code maintains invariants, but some tests are too slow to run regularly in CI or impractical to run in general, hence corresponding annotations).

Similarly no-panic ensures compiler can figure out the code can't possibly panic at compile time, which allows for better optimizations, but this repo doesn't run corresponding tests, so not sure how useful it is to port over.

I'm trying to minimise the diff with abundance, so the next backport is easier.

Some of the later commits can be merged up as they add no extra benefit for review

I'd like to keep them separate so they're easier to revert, if we need to do that in the next backport. It's also useful to have the backport steps clearly listed.

Edit: I squashed some similar commits

As at commit: nazar-pc/abundance@6d56b48

…o subspace versions

teor2345 · 2025-10-22T22:18:23Z

crates/subspace-verification/src/lib.rs


    // Check that proof of space is valid
-    if !PosTable::is_proof_valid(
+    if !<PosTable as SolutionPotVerifier>::is_proof_valid(


@nazar-pc in the subspace benchmarks, I'm getting an InvalidProofOfSpace error here:

subspace/crates/pallet-subspace/src/extensions/benchmarking.rs

Line 87 in 5450019

SubspaceExtension::<T>::do_check_vote(&signed_vote, TransactionSource::InBlock)

subspace/crates/pallet-subspace/src/extensions/benchmarking.rs

Line 137 in 5450019

SubspaceExtension::<T>::do_check_vote(&signed_vote, TransactionSource::InBlock)

I'm not sure if:

my changes in this PR have broken verification (I've checked and none of my commits should change the result)

there's an unexpected incompatibility in the abundance source commit

the benchmark is wrong in some way, and this code is stricter

the abundance code doesn't handle unlimited solution ranges

some other issue

Does anything seem obvious to you?

Nothing obvious from a quick look, but I have not audited the code. Try to start a local node and farmer, see if it can produce blocks. Even with changed logic for constructing tables, if challenge is derived the same way, verification should succeed. Something is significantly wrong if verification doesn't pass.

You can also add verification right after proof generation and dump the seed and challenge index for which verification fails. You can then update tests to check that particular seed both here and in abundance repo to see where things do not match.

Using the hard-coded vote.data, which is decoded using VoteData, I get a failure in ShimTable (only used for tests) at:

subspace/crates/subspace-proof-of-space/src/shim.rs

Line 37 in fa44a46

return false;

Which means no solution was found.

This happens because ab-proof-of-space inverted the ShimTable logic, I've reverted to the current Subspace logic, and the benchmarks pass.

Why not changing the vote data to match updated shim implementation then? Also if benchmarks are running shim implementation instead of Chia, that doesn't seem right.

Why not changing the vote data to match updated shim implementation then?

I wasn't in the mood for mucking around with binary files 😂

But seriously, if we are just going to change to the real implementation in the next PR, there's no point in changing the vote data in this one as well.

Also if benchmarks are running shim implementation instead of Chia, that doesn't seem right.

I had that thought as well. Happy to move to the real implementation in another PR.

If there's any expensive setup we can put it outside the benchmarked code. (Or in a OnceCell if it's very expensive.)

@vedhavyas @jfrank-summit what do you think?

nazar-pc and others added 3 commits October 15, 2025 15:44

Replace subspace-proof-of-space with latest compatible ab-proof-of-sp…

3a3e07b

…ace (nazar-pc/abundance#406) Using the entire ab-proof-of-space as at commit: nazar-pc/abundance@6d56b48 And merge changes from ab-proof-of-space/Cargo.toml into subspace-proof-of-space/Cargo.toml.

Fix ab_proof_of_space import names

f663670

Copy ab-chaha8 from abundance (nazar-pc/abundance#406)

9b280b6

Using the entire ab-chaha8 as at commit: nazar-pc/abundance@6d56b48

teor2345 self-assigned this Oct 15, 2025

teor2345 requested review from nazar-pc and vedhavyas as code owners October 15, 2025 13:49

teor2345 added enhancement New feature or request performance Related to performance measurement or improvement labels Oct 15, 2025

teor2345 enabled auto-merge October 15, 2025 13:50

nazar-pc reviewed Oct 15, 2025

View reviewed changes

vedhavyas reviewed Oct 22, 2025

View reviewed changes

teor2345 added 6 commits October 23, 2025 06:36

Fix ab-proof-of-space dependencies

c3398e8

Backport trait SolutionPotVerifier (nazar-pc/abundance#406)

2e4a5a1

As at commit: nazar-pc/abundance@6d56b48

Fix incompatible SBucket type in SolutionPotVerifier and fix imports

1acd4fd

Revert ab-proof-of-space compiler features and chacha20 API changes t…

cd12ae1

…o subspace versions

Fix ab-proof-of-space lints

6a0269f

Add missing module docs

fa44a46

teor2345 force-pushed the abundance-pos-backport branch from 838b880 to fa44a46 Compare October 22, 2025 22:08

teor2345 commented Oct 22, 2025

View reviewed changes

This comment was marked as resolved.

Sign in to view

teor2345 force-pushed the abundance-pos-backport branch 2 times, most recently from 2e8bf24 to 84dc9fa Compare October 24, 2025 04:10

teor2345 requested review from nazar-pc and vedhavyas October 24, 2025 04:10

teor2345 added 2 commits October 24, 2025 16:51

Revert ShimTable changes from ab-proof-of-space and add more tests

d2aeceb

Add debugging for vote benchmark errors

c019daa

teor2345 force-pushed the abundance-pos-backport branch from 84dc9fa to c019daa Compare October 24, 2025 07:06

Backport Proof of Space perf improvements from abundance #3712

Are you sure you want to change the base?

Backport Proof of Space perf improvements from abundance #3712

Uh oh!

Conversation

teor2345 commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code contributor checklist:

Uh oh!

immunefi-magnus bot commented Oct 15, 2025

🛡️ Immunefi PR Reviews

Uh oh!

teor2345 commented Oct 15, 2025

Uh oh!

nazar-pc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vedhavyas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teor2345 commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

teor2345 commented Oct 15, 2025 •

edited

Loading

nazar-pc left a comment •

edited

Loading

vedhavyas left a comment •

edited

Loading

teor2345 commented Oct 22, 2025 •

edited

Loading